-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Asset pipeline rewrite #4337
Asset pipeline rewrite #4337
Conversation
@tozz eslint isn't enforced for consuming apps |
@bcardarella Oh yeah, my bad, this is not the actual assets folder, it's just the naming of things that makes it confusing (things are named the same in source vs created project but are completely different) 😅 |
@tozz fwiw I think having the |
@chrismccord this test: https://github.com/phoenixframework/phoenix/blob/master/installer/test/phx_new_test.exs#L17-L22 |
assets/.babelrc
Outdated
@@ -2,4 +2,4 @@ | |||
"presets": [ | |||
"@babel/preset-env" | |||
] | |||
} | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it intentional to have no newlines at the end of all files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't. I bet my editor did that. I'll correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be fixed now. I added an eslint rule to catch it for the js files in the future, I don't think it will catch on this config files though.
@bcardarella Any thoughts on using webpack-merge for changing webpack configuration based on your current environment? |
@tomciopp if I am understanding the README correctly, all this plugin is doing is a left->right object merge. So this would be used to write your own custom webpack config in a different file then merge into the default? If this is the case I don't really see a need for this. Why not just change what is generated from the template? |
@bcardarella Sure you could do that, but |
@tomciopp I see what you're saying but our goal here is to reduce complexity. For the majority of users they're unlikely to need to make changes to the webpack config. For those that want to they can add this plugin if they don't want to make many changes to the generated template. The asset pipeline got into its current state because things kept getting added and after a few conversations with @chrismccord to this point we're going to keep the default template as sparse and basic as possible. Nothing prevents people from adding whatever they'd like to customize away from the default. |
I'm not so sure about the validity of this statement, based on personal experience I've found it to be a necessity for every project. YMMV. I understand your point of reducing complexity, but if you are right and the majority of users don't change the configuration, shouldn't we optimize for people who need to? This way we aren't leaving people in hell if they aren't power users of webpack and need to make changes. Perhaps a blog post would suffice for my opinionated guide for phoenix & webpack configuration for larger scale projects. |
Something like this would be great, but that's outside the scope of this PR. Recently SASS was removed and I'm sure there are some that were using it so including and a guide to things like that would be helpful too. Another guide would be on improving the build time of webpack. Prior to this PR there were some optimizations to handle slower builds but sacrificed source map fidelity. Everything is a trade off so giving some people pointers on these things would be good to have. However, I would caution that due to the complexity of webpack and the seemingly infinite permutations on webpack configs any effort on a guide of this type should probably have clearly defined boundaries. |
15a36b6
to
8be7050
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is amazing! Do we need the babel.config.json
btw?
I also checked the asset size real quick. Currently phoenix.js
is around 25kb. This PR it seems to have dropped to 20kb!
Also, what is the plan for versioning given the breaking changes?
loader: 'babel-loader' | ||
globalObject: 'this' | ||
}, | ||
devtool: devMode ? 'source-map' : undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥳
@@ -11,16 +11,13 @@ | |||
"topbar": "^0.1.4"<% end %> | |||
}, | |||
"devDependencies": { | |||
"@babel/core": "^7.0.0", | |||
"@babel/preset-env": "^7.0.0", | |||
"babel-loader": "^8.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚫 babel
|
||
module.exports = (env, options) => { | ||
const devMode = options.mode !== 'production'; | ||
console.log(`CURRENT MODE: ${devMode}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing this was intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, will rm
|
||
return { | ||
resolve: { | ||
modules: ['node_modules'] | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the default, but totally great that it is explicit!
"import": "./assets/js/phoenix.js", | ||
"require": "./priv/static/phoenix.js" | ||
".": "./assets/js/phoenix/index.js", | ||
"./": "./assets/js/phoenix/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming you found that you needed both?
The first one being import "phoenix";
. Second one any subpath such as import "phoenix/longpoll";
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if there is a pattern to do in 1 LOC I'm open to it
Implemented new pipeline with es6-ified modules and got mocha test suite green
8be7050
to
538c458
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for starting this topic. I took a quick look at the installer things, without testing or checking out this branch.
If any questions arises - please feel free.
"terser-webpack-plugin": "^2.3.2", | ||
"webpack": "4.41.5", | ||
"webpack-cli": "^3.3.2" | ||
"copy-webpack-plugin": "^9.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not see copy-webpack-plugin
beeing used anymore within webpack.config.js
.
Means it could be removed here.
But by removing copy-webpack-plugin
I think in installer/lib/phx_new/single.ex
copy path of robots.txt
, phoenix.png
and favicon.ico
should be changed from assets/static/
to priv/static/
.
With this change /priv/static
should get removed in the .gitignore
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moving them over to priv/static
might make sense, but it depends upon what experience @chrismccord wants here. If all web assets should be in the assets/
directory then copied over at build time I can bring back the copy rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to support copying assets from assets/static/
to priv/static
, so we'lll need copy-webpack unless webpack can do something like this out of the box now. This allows folks to put all assets inside the assets directory, and build them into private/static, which is especially important for our digest task, which will digest the copied/built assets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then it would be necessary to bring it back in.
I'm not aware of any simpler out of the box webpack solution.
One thing I once encountered but haven't evaluated in detail then,
copy-webpack-plugin
caused to copy (or touch all copy files) if one of them canged. Means live reload reloads x times (or logged to reload).
But could be, that this was only true for a certain version of the updated plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cw789 I'm working on putting it back right now, the API changed with the version bump so I just have to ensure that it's doing everything we want.
"copy-webpack-plugin": "^9.0.0", | ||
"css-loader": "^5.2.6", | ||
"css-minimizer-webpack-plugin": "^3.0.0", | ||
"expose-loader": "3.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expose-loader
not being used within webpack.config.js
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left it in because @chrismccord and I were going back and forth on if the Phoenix
constant should be exposed. He fell on the side that it wasn't necessary but I wanted to give the community a chance to provide feedback if for some reason a need for that constant is being overlooked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear, the Phoenix
constant needs to be on window when the built js is loaded, for folks not using an asset builder and for the live reload project. If we can ditch expose-loader
and do this directly then great, but we need to make sure it is exposed from the bundle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could technically be done without expose-loader
but I think it s going to be some crazy rule writing. I would vote to keep the dependency to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear, keeping it on the internal webpack config, not the one in the installer template
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
works for me
❤️❤️❤️🐥🔥 |
@bcardarella @chrismccord Is there a reason |
@gcauchon babel is being used for the test suite for the js files |
FWIW if anyone want to PR to have the js test suite use Webpack to resolve the es6 modules so we can drop Babel there too I'm all for it. I just gave up fighting JS that day I think. |
This is a comprehensive rewrite of the Phoenix asset pipeline. Closes #4328
LiveView has a similar pending PR: phoenixframework/phoenix_live_view#1474
This does include breaking changes for existing apps with legacy Webpack configurations. Upgrade instructions:
package.json
frominstaller/templates/phx_assets
to overwrite the existingpackage.json
into your app'sassets/
directory.cd assets && rm -rf node_modules && rm package-lock.json
npm install
webpack.config.js
frominstaller/templates/phx_assets
to overwrite the existingwebpack.config.js
in yourassets/
directoryNote if you have any customizations in your existing
webpack.config.js
you should port them to the new config. This PR includes an update from Webpack v4 to Webpack v5 so there may be incompatibilities with any plugins, loaders, etc... that you were using web Webpack v4By default the asset pipeline will import the es6 modules for Phoenix (as well as Phoenix LiveView). This means it is no longer building from pre-compiled JS. The result is that in dev mode the Phoenix and LiveView JS is no longer minified and no longer uglified. Debugging and stepping through code at runtime is now much easier. In production the JavaScript is minified and uglified as well as the CSS.
Babel has been removed from the build pipeline for apps. This was done because the target minimum browser version for Phoenix is being bumped up. Targeting IE11 is no longer in scope and we no longe need to include the Babel polyfill anymore. If you still require Babel for your own app's needs you can easily add Babel to your
package.json
and add the correct rules/loaders towebpack.config.js
.Sourcemaps are now included out of the box. The previous sourcemap implementation favored compilation speed over how useful they were. Now sourcemaps are generated and can be accessible in the developer console:
TODO for Sourcemaps: the naming of the asset files needs to be improved. They currently compile with their relative path names from the Phoenix and Phoenix LiveView directories (i.e. "js/phoenix/channel.js") This should be improved to just be "phoenix/channel.js"
ESlint was added with a set of rules that attempt to conform to @chrismccord's desired JS style. Currently the linter is not run with the test suite. You can run from the
assets/
directory:npx eslint js
and you can add--fix
if you trust the linter to auto-fix anything that isn't compliant. I do think that the linter should be part of the test suite to help prevent any future style fix requests on PRsThere should not be any changes necessary to consuming app's
app.js
to consume the new es6 modules. I made sure to export the necessary modules properly.TBD
Anyone not using webpack please weigh in on this. The changes I've made were with webpack in mind. I removed quite a bit that you may be taking for granted. Let's discuss that. While adding complexity to this PR can be done it isn't desirable. Webpack is being seen as the anointed asset pipeline solution for Phoneix. If you are using something else I believe a npm module to auto-configure what you need is the best step forward (i.e. creating a
phoenix-rollup
package) rather than adding to what Phoenix generates. However, if there is something missing that is an absolute blocker necessary to build in your own environment let's discuss what that is and how best to support.TODO
priv/static/phoenix.js
should be a production build and not include any references to source maps.